Repository navigation
shell: route FIFO redirect targets through the pollable IOWriter path - #34696
Conversation
Redirecting a builtin's stdout to a named pipe (echo ... > fifo) opened the target via open_for_writing_impl, which correctly reported pollable=true for S_IFIFO and set O_NONBLOCK on the fd, but Builtin then discarded that result and hardcoded pollable=false for the IOWriter on POSIX. The writer therefore took the synchronous file path, whose drain loop treats EAGAIN as unreachable. With a slow or paused reader the pipe buffer fills, write(2) returns EAGAIN, and the process aborts with: panic: internal error: entered unreachable code: drainBufferedData returning .pending in IOWriter.doFileWrite should not happen Pass the computed pollable flag through so FIFO/socket redirect targets use the pollable writer (same as pipeline stdout already does), and replace the unreachable in do_file_write with an in-place upgrade to the pollable path so any remaining misclassification surfaces as backpressure rather than an abort.
|
Reproduced on 1.4.0 and main. PR #34696 routes FIFO redirect targets through the pollable writer and replaces the CI: the diff is green. The new test passed on every lane across builds #75740 and #75742. Remaining reds are unrelated: |
|
Updated 5:52 AM PT - Jul 19th, 2026
❌ @robobun, your commit eaec13d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34696That installs a local version of the PR into your bun-34696 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesThe shell now preserves computed file-writer pollability on POSIX, retries pending synchronous writes through a nonblocking path, and adds a POSIX FIFO backpressure test. FIFO redirection handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/shell/IOWriter.rs`:
- Around line 719-738: Extract the duplicated partial-write accounting sequence
from the Pending branch and the nearby Done/Wrote path into a small helper on
the IOWriter implementation. Have both paths use the helper to update the tee
buffer, total_bytes_written, and per-writer written count consistently, while
preserving their existing control flow and outcome handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2572505e-a686-4e3a-a0e8-977c0212ce76
📒 Files selected for processing (3)
src/runtime/shell/Builtin.rssrc/runtime/shell/IOWriter.rstest/js/bun/shell/file-io.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this touches the shell IOWriter's state machine (a new mid-write upgrade from the sync file path to the pollable path, with UnsafeCell re-entrancy and Stacked Borrows discipline in play), so a human should sign off.
What was reviewed:
Builtin.rs: verifiedopen_for_writing_implalready setspollable/is_nonblockingforS_IFIFO/S_IFSOCKand that no otherIOWriter::initcaller hardcodespollable=falsefor a nonblocking fd (Pipeline usestrue, interpreter usesis_pollable(fd)).do_file_writePendingarm: traced thatrecord_write_progressadvancestotal_bytes_written/writtenso the subsequentget_buffer()slices the correct remainder;Pending(amt)guaranteesamt < lenso thedebug_assert!(len != written)inget_buffer_implholds; the__start()EINVAL/EPERM fallback resetsflags.pollable=falsebeforeIsActuallyFileis returned, so itsdebug_assert!(!pollable)still holds.- Test: pre-fill via
wfdguarantees the shell's first write hits EAGAIN; final drain runs afterawait pending+closeSync(wfd)so all 'z' bytes are countable; parentholderfd is read-only and never drained, so it can't steal bytes from the child reader.
Extended reasoning...
Overview
Three files: Builtin.rs (pass computed pollable instead of hardcoded false to the redirect IOWriter on POSIX), IOWriter.rs (replace the unreachable!() on WriteResult::Pending in do_file_write with an in-place upgrade to the pollable path, plus a record_write_progress helper extracted per CodeRabbit feedback), and a new POSIX-gated FIFO backpressure test in file-io.test.ts.
Security risks
None. This is internal shell fd-classification and write-path selection; no user input parsing, auth, or crypto.
Level of scrutiny
Medium-high. IOWriter is a hand-rolled UnsafeCell<State> state machine with explicit Stacked Borrows re-derive discipline and re-entrancy through Yield::run. The new Pending arm introduces a state transition (sync-file → pollable) that did not exist before: it mutates flags.pollable/nonblock/started mid-do_file_write and calls self.write() while the scopeguard::defer for set_writing(false) is still armed. On POSIX set_writing is a no-op so this is benign, and the existing __start() EINVAL/EPERM fallback correctly resets flags.pollable=false before IsActuallyFile is returned (so the debug_assert!(!s.flags.pollable) in write() still holds and the caller surfaces EAGAIN via on_sync_error rather than looping). I traced these paths and they look correct, but the interaction surface is subtle enough that a maintainer familiar with the IOWriter re-entrancy model should confirm.
Other factors
- The Builtin.rs one-liner is the actual root-cause fix; the IOWriter.rs change is defense-in-depth (with the Builtin fix, a FIFO redirect target now takes the pollable path from the start and never enters
do_file_write). I checked the other threeIOWriter::initcall sites (interpreter.rs stdout/stderr, Pipeline.rs) — none hardcodepollable=falsefor a nonblocking fd, so this was the only site with the bug. Pending(amt)fromdrain_buffered_dataguaranteesamt < len(the innertry_write_with_write_fnloop exits withWroteonceoffset == buf.len()), so afterrecord_write_progressthe writer is not yet complete and the follow-onget_buffer_implslice is non-empty and itsdebug_assert!(len != written)holds.- Test looks solid: pre-fills the pipe so EAGAIN is deterministic, counts exact 'z' bytes rather than length, drains concurrently via
setImmediate(no sleep), and holds a parent-side read fd so a crashing child doesn't wedge tempDir teardown. VerifiedUSE_SYSTEM_BUN=1fails per the PR body. - CodeRabbit's dedup nit was addressed in 85c00e9 and the thread is resolved; no outstanding review comments.
|
The error-path-drops-live-owner audit hardening items (keepalive in |
There was a problem hiding this comment.
No bugs found, but this reworks the shell IOWriter's non-pollable → pollable transition (poll re-registration, UnsafeCell state re-derives, sync-error completion routing) and the author flagged an overlapping change in #34697 — worth a human maintainer confirming the merge order and the started=false/__start() restart interaction.
What was reviewed:
Builtin.rsnow forwardsopen_for_writing_impl's computedpollableinstead of hardcodingfalse; regular files still getpollable=falsefromis_pollable_from_mode.do_file_write's newPendingarm records progress before the restart, soget_buffer()returns the unwritten remainder;record_write_progressre-derivesstate()fresh so no overlapping&mut State.- The
IsActuallyFilefallback surfaces EAGAIN viaon_sync_error(returned to the enqueuing trampoline) rather than looping or panicking. - Test is POSIX-gated, pre-fills the FIFO to force EAGAIN on the shell's first write, drains concurrently, asserts exact 'z' count; cleanup via
using/try-finally.
Extended reasoning...
Overview
Fixes a deterministic panic when a shell builtin redirects to a FIFO/socket by path (echo big > fifo). Two changes: (1) Builtin.rs stops discarding the pollable out-param from open_for_writing_impl and passes it through to IOWriter::init, so FIFO/socket targets take the poll-driven path from the start; (2) IOWriter::do_file_write replaces the unreachable!() on WriteResult::Pending with an in-place upgrade to the pollable path (record partial write, flip flags.pollable/nonblock, reset started, call write()), degrading to an EAGAIN error via on_sync_error if the fd also refuses epoll registration. A shared record_write_progress helper was extracted per CodeRabbit's nit. A new test in file-io.test.ts pre-fills a FIFO's pipe buffer so the shell's first write hits EAGAIN deterministically, then drains and counts payload bytes.
Security risks
None identified. No untrusted-input parsing, no auth/crypto, no path traversal surface. The change only affects how an already-opened redirect fd is written to.
Level of scrutiny
Medium-high. This is native runtime code in the shell's I/O state machine, sitting on top of UnsafeCell<State> with hand-managed Stacked Borrows discipline (multiple explicit re-derive comments in the surrounding code). The Pending → pollable upgrade introduces a new state transition (started=false after having been true, then __start() re-registers a poll on an fd whose writer.handle was never previously set by the non-pollable path). It looks correct against PosixBufferedWriter::start, and the test exercises it end-to-end, but the interaction between enqueue_file (which sets started=true without touching writer.handle) and the restart deserves a maintainer's confirmation. Given the primary fix in Builtin.rs means FIFOs no longer reach do_file_write in the first place, the upgrade is defense-in-depth — but it's still new control flow in a subtle area.
Other factors
- The bug hunting system found no issues; CodeRabbit's only comment (dedup the accounting bump) was addressed in 85c00e9.
- Evidence gate shows the test fails on the unfixed build (panic, exit 134) and passes with the fix on both ASAN debug and release.
- The author noted overlapping
Builtin.rschanges in PR #34697 (error-path keepalive audit) and offered to rebase either way — a human should decide merge order. - Test follows harness conventions:
tempDir,bunEnvspread, concurrent pipe drain viaPromise.all,using/try-finally cleanup, POSIX skip guard, no timing sleeps. - Not approving because this is not a simple/mechanical change: it modifies a core I/O state transition in native code with explicit aliasing invariants.
Repro
Deterministic on Linux and macOS.
Cause
Builtinopens a>-redirect target viaopen_for_writing_impl, which fstats the fd, reportspollable = trueforS_IFIFO/S_IFSOCK, and setsO_NONBLOCK. On POSIX the caller then discarded that result and passed a hardcodedpollable = falsetoIOWriter::init, so the writer took the synchronous file path (do_file_write). That path driveswrite(2)in a loop and treatsWriteResult::Pending(EAGAIN) asunreachable!()on the assumption that a "file" never returns EAGAIN. A FIFO with a slow reader does: once the pipe buffer fills, the next write returns EAGAIN and the process aborts. Pipeline stdout (echo big | slow) already went through the pollable writer and handled the same fd shape correctly.Fix
Builtin.rs: pass thepollablevalue computed byopen_for_writing_implto the redirectIOWriteron POSIX (Windows keeps its unconditional async writer). FIFO/socket redirect targets now take the pollable path and poll for writability under backpressure.IOWriter::do_file_write: replace theunreachable!()forPendingwith an in-place upgrade: record the partial write, flipflags.pollable/nonblock, and restart viawrite()so the remainder drains through the poll machinery. If the fd turns out to also be un-epollable, the EAGAIN is surfaced as the chunk's write error instead of aborting.Verification
The full
file-io.test.ts(27 tests) andbunshell.test.ts(412 tests) suites pass.[review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file